[SPARK-19080][SQL] simplify data source analysis #16269

cloud-fan · 2016-12-13T18:43:50Z

What changes were proposed in this pull request?

The current way of resolving InsertIntoTable and CreateTable is convoluted: sometimes we replace them with concrete implementation commands during analysis, sometimes during planning phase.

And the error checking logic is also a mess: we may put it in extended analyzer rules, or extended checking rules, or CheckAnalysis.

This PR simplifies the data source analysis:

InsertIntoTable and CreateTable are always unresolved and need to be replaced by concrete implementation commands during analysis.
The error checking logic is mainly in 2 rules: PreprocessTableCreation and PreprocessTableInsertion.

How was this patch tested?

existing test.

SparkQA · 2016-12-13T21:03:10Z

Test build #70092 has finished for PR 16269 at commit c075191.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
case class CreateTable(
case class PreprocessTableCreation(conf: SQLConf) extends Rule[LogicalPlan]
class HiveAnalysis(session: SparkSession) extends Rule[LogicalPlan]

cloud-fan · 2017-01-04T14:55:27Z

cc @yhuai @gatorsmile

SparkQA · 2017-01-04T14:58:39Z

Test build #70878 has finished for PR 16269 at commit e94b187.

This patch fails RAT tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class CreateTableCommand(
case class PreprocessTableCreation(sparkSession: SparkSession) extends Rule[LogicalPlan]
class HiveAnalysis(session: SparkSession) extends Rule[LogicalPlan]

SparkQA · 2017-01-04T15:05:10Z

Test build #70880 has finished for PR 16269 at commit 483c31f.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class CreateTableCommand(
case class PreprocessTableCreation(sparkSession: SparkSession) extends Rule[LogicalPlan]
class HiveAnalysis(session: SparkSession) extends Rule[LogicalPlan]

SparkQA · 2017-01-04T15:31:11Z

Test build #70882 has finished for PR 16269 at commit 6810dcf.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class CreateTableCommand(
case class PreprocessTableCreation(sparkSession: SparkSession) extends Rule[LogicalPlan]
class HiveAnalysis(session: SparkSession) extends Rule[LogicalPlan]

SparkQA · 2017-01-04T17:24:23Z

Test build #70884 has finished for PR 16269 at commit 87be209.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class CreateTableCommand(
case class PreprocessTableCreation(sparkSession: SparkSession) extends Rule[LogicalPlan]
class HiveAnalysis(session: SparkSession) extends Rule[LogicalPlan]

SparkQA · 2017-01-11T06:43:49Z

Test build #71188 has started for PR 16269 at commit 87be209.

SparkQA · 2017-01-23T09:40:27Z

Test build #71834 has finished for PR 16269 at commit a73134e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class CreateTableCommand(
case class PreprocessTableCreation(sparkSession: SparkSession) extends Rule[LogicalPlan]

SparkQA · 2017-01-23T14:43:20Z

Test build #71839 has finished for PR 16269 at commit 3336699.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class CreateTableCommand(
case class PreprocessTableCreation(sparkSession: SparkSession) extends Rule[LogicalPlan]

SparkQA · 2017-01-23T14:47:59Z

Test build #71840 has finished for PR 16269 at commit cede3c9.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-01-23T19:22:26Z

Test build #71861 has finished for PR 16269 at commit 771a36b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-01-23T19:23:39Z

Test build #71862 has finished for PR 16269 at commit dcea3a6.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-01-23T21:13:44Z

Will read it carefully tomorrow. Thanks for the great work!

SparkQA · 2017-01-24T07:10:15Z

Test build #71908 has finished for PR 16269 at commit 4b68c16.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-01-24T15:34:32Z

Test build #71932 has finished for PR 16269 at commit 48535aa.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class HiveFileFormat(fileSinkConf: FileSinkDesc)

SparkQA · 2017-02-03T14:36:19Z

Test build #72306 has finished for PR 16269 at commit 55a1db3.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-02-03T18:08:31Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

-              t.isInstanceOf[Range] ||
-              t == OneRowRelation ||
-              t.isInstanceOf[LocalRelation] =>
-            failAnalysis(s"Inserting into an RDD-based table is not allowed.")


To other reviewers: This is moved to PreWriteCheck

gatorsmile · 2017-02-03T18:12:04Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

+    plan.foreachUp {
+      case o if !o.resolved => failAnalysis(s"unresolved operator ${o.simpleString}")
+      case _ =>
+    }


This movement is great, since extendedCheckRules could output a better error message.

gatorsmile · 2017-02-03T18:16:57Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

-                  s"column(s) but the inserted data has " +
-                  s"${query.output.size + numStaticPartitions} column(s), including " +
-                  s"$numStaticPartitions partition column(s) having constant value(s).")
-            }


To other reviewers: This is moved to PreprocessTableInsertion.

After the above two moves, def checkAnalysis(plan: LogicalPlan) does not have any DDL/DML error handling. It becomes cleaner

gatorsmile · 2017-02-03T18:24:16Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

 /**
- * Insert some data into a table.
+ * Insert some data into a table. Note that this plan is unresolved and has to be replaced by the
+ * concrete implementations during analysis.


To other reviewers, InsertIntoTable is a unified unresolved node for representing INSERT. After completing the resolution, it will be replaced to InsertIntoDataSourceCommand, InsertIntoHadoopFsRelationCommand or InsertIntoHiveTable.

gatorsmile · 2017-02-03T18:27:21Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala

-    def apply(plan: LogicalPlan): Seq[SparkPlan] = plan match {
-      case CreateTable(tableDesc, mode, None) if DDLUtils.isHiveTable(tableDesc) =>
-        val cmd = CreateTableCommand(tableDesc, ifNotExists = mode == SaveMode.Ignore)
-        ExecutedCommandExec(cmd) :: Nil


To other reviewers: this is moved to HiveAnalysis.

gatorsmile · 2017-02-03T18:28:40Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala

-            tableDesc,
-            mode,
-            query)
-        ExecutedCommandExec(cmd) :: Nil


To other reviewers: the above two are moved to DataSourceAnalysis

gatorsmile · 2017-02-03T18:29:48Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala

-            query)
-        ExecutedCommandExec(cmd) :: Nil
-
-      case c: CreateTempViewUsing => ExecutedCommandExec(c) :: Nil


To other reviewers: Since CreateTempViewUsing is a RunnableCommand, this line is useless

gatorsmile · 2017-02-03T18:32:36Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala


-    case InsertIntoTable(l @ LogicalRelation(t: InsertableRelation, _, _),
-      part, query, overwrite, false) if part.isEmpty =>
-      ExecutedCommandExec(InsertIntoDataSourceCommand(l, query, overwrite)) :: Nil


To the other reviewers: this is moved to DataSourceAnalysis

gatorsmile · 2017-02-03T18:43:41Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/ddl.scala


+/**
+ * Create a table and optionally insert some data into it. Note that this plan is unresolved and
+ * has to be replaced by the concrete implementations during analyse.


To the other reviewers, CreateTable is a unified unresolved logical representation of CREATE TABLE (AS SELECT). It will be replaced by CreateHiveTableAsSelectCommand, CreateTableCommand, CreateDataSourceTableCommand or CreateDataSourceTableAsSelectCommand.

gatorsmile · 2017-02-03T18:46:08Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

+  // We don't want `table` in children as sometimes we don't want to transform it.
+  override def children: Seq[LogicalPlan] = query :: Nil
+  override def output: Seq[Attribute] = Seq.empty
+  override lazy val resolved: Boolean = false


@cloud-fan After this change, we are unable to reach the check in checkForStreaming. BTW, it sounds like we do not have any test case to cover these scenarios.

https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/UnsupportedOperationChecker.scala#L110-L115

now we will resolve CreateTable, InsertIntoTable to concrete commands, so the check can still work.

gatorsmile · 2017-02-03T18:48:58Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/ddl.scala


+/**
+ * Create a table and optionally insert some data into it. Note that this plan is unresolved and
+ * has to be replaced by the concrete implementations during analyse.


Nit: analyse -> analysis

gatorsmile · 2017-02-03T19:05:30Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveStrategies.scala

 *
- * Note that, this rule must be run after `PreprocessTableInsertion`.
+ * Note that, this rule must be run after `PreprocessTableCreation` and
+ * `PreprocessTableInsertion`.


Could you also add two more rules OrcConversions and ParquetConversions? HiveAnalysis must be run after them too.

gatorsmile · 2017-02-03T19:05:45Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala

 *
- * Note that, this rule must be run after [[PreprocessTableInsertion]].
+ * Note that, this rule must be run after `PreprocessTableCreation` and
+ * `PreprocessTableInsertion`.


The same updates are needed here.

gatorsmile · 2017-02-03T19:07:41Z

LGTM except a few minor comments. The major concern is about our error checking for structured streaming. It sounds like the test case coverage in that area is weak.

gatorsmile · 2017-02-06T13:38:11Z

LGTM pending test

SparkQA · 2017-02-06T15:28:13Z

Test build #72444 has finished for PR 16269 at commit 9742f78.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2017-02-06T16:37:30Z

thanks for the review, merging to master!

## What changes were proposed in this pull request? The current way of resolving `InsertIntoTable` and `CreateTable` is convoluted: sometimes we replace them with concrete implementation commands during analysis, sometimes during planning phase. And the error checking logic is also a mess: we may put it in extended analyzer rules, or extended checking rules, or `CheckAnalysis`. This PR simplifies the data source analysis: 1. `InsertIntoTable` and `CreateTable` are always unresolved and need to be replaced by concrete implementation commands during analysis. 2. The error checking logic is mainly in 2 rules: `PreprocessTableCreation` and `PreprocessTableInsertion`. ## How was this patch tested? existing test. Author: Wenchen Fan <[email protected]> Closes apache#16269 from cloud-fan/ddl.

cloud-fan force-pushed the ddl branch from c075191 to e94b187 Compare January 4, 2017 14:53

cloud-fan changed the title ~~[WIP] simplify data source analysis~~ [SPARK-19080][SQL] simplify data source analysis Jan 4, 2017

cloud-fan force-pushed the ddl branch from e94b187 to 483c31f Compare January 4, 2017 15:03

cloud-fan force-pushed the ddl branch from 483c31f to 6810dcf Compare January 4, 2017 15:21

cloud-fan force-pushed the ddl branch from 6810dcf to 87be209 Compare January 4, 2017 15:53

add post-hoc resolution

c55a1f9

cloud-fan force-pushed the ddl branch from 87be209 to a73134e Compare January 23, 2017 09:05

simplify data source analysis

3336699

cloud-fan force-pushed the ddl branch from a73134e to 3336699 Compare January 23, 2017 10:56

Merge remote-tracking branch 'origin/master' into ddl

cede3c9

fix test

dcea3a6

cloud-fan force-pushed the ddl branch from 771a36b to dcea3a6 Compare January 23, 2017 16:54

cloud-fan added 2 commits January 24, 2017 12:27

Merge remote-tracking branch 'origin/master' into ddl

19e4924

update

4b68c16

cloud-fan force-pushed the ddl branch from 95e140e to 4b68c16 Compare January 24, 2017 04:33

Merge remote-tracking branch 'origin/master' into ddl

48535aa

Merge remote-tracking branch 'origin/master' into ddl

55a1db3

gatorsmile reviewed Feb 3, 2017

View reviewed changes

cloud-fan added 2 commits February 6, 2017 16:56

Merge remote-tracking branch 'origin/master' into ddl

6274f2b

address comments

9742f78

asfgit closed this in aff5302 Feb 6, 2017

[SPARK-19080][SQL] simplify data source analysis #16269

[SPARK-19080][SQL] simplify data source analysis #16269

Uh oh!

Conversation

cloud-fan commented Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Dec 13, 2016

Uh oh!

cloud-fan commented Jan 4, 2017

Uh oh!

SparkQA commented Jan 4, 2017

Uh oh!

SparkQA commented Jan 4, 2017

Uh oh!

SparkQA commented Jan 4, 2017

Uh oh!

SparkQA commented Jan 4, 2017

Uh oh!

SparkQA commented Jan 11, 2017

Uh oh!

SparkQA commented Jan 23, 2017

Uh oh!

SparkQA commented Jan 23, 2017

Uh oh!

SparkQA commented Jan 23, 2017

Uh oh!

SparkQA commented Jan 23, 2017

Uh oh!

SparkQA commented Jan 23, 2017

Uh oh!

gatorsmile commented Jan 23, 2017

Uh oh!

SparkQA commented Jan 24, 2017

Uh oh!

SparkQA commented Jan 24, 2017

Uh oh!

SparkQA commented Feb 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gatorsmile commented Feb 6, 2017

Uh oh!

cloud-fan commented Dec 13, 2016 •

edited

Loading

gatorsmile Feb 3, 2017 •

edited

Loading

gatorsmile Feb 3, 2017 •

edited

Loading

gatorsmile Feb 3, 2017 •

edited

Loading

gatorsmile commented Feb 3, 2017 •

edited

Loading